Add HistoryEventSerializationBinder to constrain HistoryEvent deserialization#1343
Closed
Add HistoryEventSerializationBinder to constrain HistoryEvent deserialization#1343
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a restricted JSON serialization binder for Azure Table–backed orchestration history event deserialization in the ServiceBus backend to reduce unsafe $type-based deserialization exposure.
Changes:
- Introduced
HistoryEventSerializationBinderto allowlist deserializable types for history event payloads. - Updated Azure Table history entity deserialization to use the new binder.
- Added MSTest coverage validating round-trip behavior and rejection of non-allowlisted
$typevalues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/DurableTask.ServiceBus/Tracking/HistoryEventSerializationBinder.cs | New restrictive binder (extends PackageUpgradeSerializationBinder) that rejects non-allowlisted resolved types. |
| src/DurableTask.ServiceBus/Tracking/AzureTableOrchestrationHistoryEventEntity.cs | Switches history event read settings to use the new restrictive binder. |
| Test/DurableTask.ServiceBus.Tests/HistoryEventSerializationBinderTests.cs | Adds tests for round-trip tags and $type rejection/allow scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AnatoliB
force-pushed
the
anatolib/codeql-fix-37181649
branch
from
25d2db1 to
b168f20
Compare
…s for improved security
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Stage 2: delegate to PackageUpgradeSerializationBinder for the legacy | ||
| // DurableTask.* -> DurableTask.Core.* rewrite and standard type resolution. | ||
| Type resolved = base.BindToType(assemblyName, typeName); |
Comment on lines
+82
to
+92
| static bool IsAllowed(Type type) | ||
| { | ||
| // Allow any type defined in DurableTask.Core (HistoryEvent subclasses, OrchestrationInstance, | ||
| // ParentInstance, FailureDetails, OrchestrationExecutionContext, etc.), plus | ||
| // Dictionary<string, string> to round-trip the IDictionary<string, string> Tags members | ||
| // declared on ExecutionStartedEvent / SubOrchestrationInstanceCreatedEvent / | ||
| // TaskScheduledEvent (Newtonsoft.Json emits a $type for these because the static | ||
| // declared type is an interface). | ||
| return type.Assembly == DurableTaskCoreAssembly | ||
| || type == typeof(Dictionary<string, string>); | ||
| } |
Comment on lines
+25
to
+33
| [TestClass] | ||
| public class HistoryEventSerializationBinderTests | ||
| { | ||
| // Mirrors the production read settings on AzureTableOrchestrationHistoryEventEntity. | ||
| static readonly JsonSerializerSettings ReadJsonSettings = new JsonSerializerSettings | ||
| { | ||
| TypeNameHandling = TypeNameHandling.Objects, | ||
| SerializationBinder = new HistoryEventSerializationBinder() | ||
| }; |
- Reject null/empty assembly names in BindToType to avoid NRE from base PackageUpgradeSerializationBinder when $type lacks an assembly portion. - Broaden assembly allowlist to host assemblies of common IDictionary<string,string> implementations (Dictionary, SortedDictionary, ConcurrentDictionary, ReadOnlyDictionary) so historical Tags persisted with non-Dictionary runtime types continue to deserialize after public APIs accept any IDictionary<string,string>. - Replace exact-Dictionary<string,string> post-check with IDictionary<string,string>-shape check; gadget types in the same BCL assembly (e.g., FileInfo) are still rejected. - Add tests: RejectsNullAssemblyName, RejectsNonBclDictionaryAssembly, AllowsSortedDictionaryStringStringForTags. Update RejectsNonAllowlistedNestedType to use Hashtable (BCL but non-IDictionary<string,string>) to exercise the post-resolution shape filter.
The ServiceBus test csproj lives at test/DurableTask.ServiceBus.Tests, but the file was committed under Test/ (capital T). On case-sensitive filesystems (Linux CI) the SDK glob rooted at the lowercase csproj folder would not pick this file up, meaning the new binder tests would not run in CI.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| private static readonly JsonSerializerSettings ReadJsonSettings = new JsonSerializerSettings | ||
| { | ||
| // CodeQL [SM02211] Constrained by HistoryEventSerializationBinder allowlist; only DurableTask.Core types and Dictionary<string,string> are accepted. |
Comment on lines
+106
to
+114
| // For all other allowlisted assemblies (the BCL dictionary hosts plus the legacy | ||
| // pre-v2 DTFx assemblies), only types assignable to IDictionary<string, string> | ||
| // are accepted. This narrows the resolved set so that gadget types living in the | ||
| // same allowlisted BCL assembly (e.g., FileInfo in System.Private.CoreLib) cannot | ||
| // pass the post-resolution check. Tags is the only IDictionary<string, string> | ||
| // member declared on a serialized HistoryEvent subtree, so any other concrete | ||
| // type is unreachable by the legitimate serializer in any case. | ||
| return typeof(IDictionary<string, string>).IsAssignableFrom(type); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves https://msazure.visualstudio.com/Antares/_workitems/edit/37181649